Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graphql: duplicate /health in GraphQL /admin #4768

Merged
merged 14 commits into from
Feb 14, 2020

Conversation

MichaelJCompton
Copy link
Contributor

@MichaelJCompton MichaelJCompton commented Feb 13, 2020

Note that the intention here is to keep /health, not replace it.

This change is Reviewable

Docs Preview: Dgraph Preview

@MichaelJCompton MichaelJCompton added the area/graphql Issues related to GraphQL support on Dgraph. label Feb 13, 2020
@MichaelJCompton MichaelJCompton marked this pull request as ready for review February 13, 2020 08:43
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 9 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain and @MichaelJCompton)


graphql/admin/admin.go, line 57 at r2 (raw file):

	}
	  
	"""Node state is the state of an individual node int he Dgraph cluster """

in the


graphql/e2e/common/common.go, line 598 at r2 (raw file):

}

func hasCurrentGraphQLSchema(url string) (bool, error) {

Wondering if we could just have one return parameter given how this is used.


graphql/resolve/resolver.go, line 497 at r2 (raw file):

// injectAliasCompletion takes a result with names as per the type names and swaps those for
// any aliases specified in the query before apply cf.

I don't completely understand this. Do we have a test which showcases this? Is it just about returning the query result along with any aliases that are specified?

Copy link
Contributor Author

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @golangcibot, @manishrjain, and @pawanrawal)


graphql/admin/admin.go, line 57 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

in the

Done.


graphql/e2e/common/admin.go, line 276 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

G107: Potential HTTP request made with variable url (from gosec)

Done.


graphql/e2e/common/common.go, line 598 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Wondering if we could just have one return parameter given how this is used.

nah, we differentiate between error, false - there's no schema and true - there is a schema


graphql/resolve/resolver.go, line 497 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I don't completely understand this. Do we have a test which showcases this? Is it just about returning the query result along with any aliases that are specified?

Yeah, the processing that goes through Dgraph handles aliases by injecting them into the Dgraph query, but the admin functions don't have that. So they all need post processing to handle the aliases.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @golangcibot, @manishrjain, and @pawanrawal)

@MichaelJCompton MichaelJCompton merged commit 2fa518b into master Feb 14, 2020
@MichaelJCompton MichaelJCompton deleted the mjc/graphql-gqlhealth branch February 14, 2020 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

3 participants